Refactor MappingElasticsearchConverter #1677

mp911de · 2021-02-01T11:10:12Z

Add support for SpEL expressions via @Value.
Simplify readCollectionOrArray to consider properly nested lists and maps
Simplify readMap to allow reading generic maps and entities in maps

This allows adding Document to ElasticsearchSimpleTypes.
Also, update DefaultElasticsearchTypeMapper to report a fallback TypeInformation to properly convert nested maps.

Closes #1676.

sothawo

two minor remarks

sothawo · 2021-02-01T20:24:26Z

src/main/java/org/springframework/data/elasticsearch/core/convert/GeoConverters.java

@@ -67,14 +68,14 @@
 	 * {@link Converter} to write a {@link Point} to {@link Map} using {@code lat/long} properties.
 	 */
 	@WritingConverter
-	public enum PointToMapConverter implements Converter<Point, Map<String, Object>> {
+	public enum PointToMapConverter implements Converter<Point, Document> {


shouldn't we rename all these converters from MapTo... and ...ToMap into DocumentTo...and ...ToDocument?

Good point. Let's do that for the next major version as converters are public classes.

sothawo · 2021-02-01T20:25:03Z

.../java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

+	 * Extension of {@link SpELExpressionParameterValueProvider} to recursively trigger value conversion on the raw
+	 * resolved SpEL value.
+	 *
+	 * @author Mark Oaluch


Suggested change

* @author Mark Oaluch

* @author Mark Paluch

sothawo · 2021-02-02T06:24:58Z

But these changes do not remove the messages on startup:

2021-02-02 07:20:06.515  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.geo.Point as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.515  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.515  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to interface org.springframework.data.elasticsearch.core.geo.GeoJson as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.515  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonLineString as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiLineString as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonPolygon as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPolygon to interface java.util.Map as writing converter although it doesn't convert to a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPolygon as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.516  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonGeometryCollection as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.637  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.geo.Point as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.637  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to interface org.springframework.data.elasticsearch.core.geo.GeoJson as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPoint as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonLineString as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiLineString as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonPolygon as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPolygon to interface java.util.Map as writing converter although it doesn't convert to a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.638  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonMultiPolygon as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.
2021-02-02 07:20:06.639  WARN 80774 --- [           main] o.s.data.convert.CustomConversions       : Registering converter from interface java.util.Map to class org.springframework.data.elasticsearch.core.geo.GeoJsonGeometryCollection as reading converter although it doesn't convert from a store-supported type! You might want to check your annotation setup at the converter implementation.

mp911de · 2021-02-02T09:02:25Z

It seems, that we need to add Map as well as simple type. That change requires a bit more rewrite of the converter.

mp911de · 2021-02-02T09:19:52Z

Actually, this change is quite useful as some components in the Template API lookup Collections.singletonMap(…), LinkedHashMap etc. as PersistentEntity which is wrong in the first place.

sothawo · 2021-02-02T09:21:40Z

important is, that the tests still run, we have some places where people read and write generic/dynamic lists, maps and stuff; these known cases are covered by tests, so as long as they don't break

mp911de · 2021-02-02T09:25:13Z

I had to update one test as the top-level document now writes a _class attribute. We can discuss whether that makes sense or not.

mp911de · 2021-02-02T09:28:55Z

I decided to revert the converters change. They now are returning Map (as previously, so we can drop both commits regarding GeoConverters) as the converter rewrite allows now to consider Map as simple type.

sothawo · 2021-02-02T09:39:32Z

I'll check after work today

sothawo

one comment about the test change; other wise to merge.

sothawo · 2021-02-02T19:00:06Z

.../springframework/data/elasticsearch/core/convert/MappingElasticsearchConverterUnitTests.java

@@ -211,6 +211,7 @@ public void init() {
 		shotGunAsMap.put("_class", ShotGun.class.getName());

 		notificationAsMap = Document.create();
+		notificationAsMap.put("_class", Notification.class.getName());


hm, that change is not really necessary as line 221 (old) already has this. Although using the class to get the name instead of the string is better.

* Add support for SpEL expressions via @value. * Simplify readCollectionOrArray to consider properly nested lists and maps * Simplify readMap to allow reading generic maps and entities in maps. * Report a fallback TypeInformation in DefaultElasticsearchTypeMapper to properly convert nested maps. We now no longer rely on isSimpleType when writing Maps. This is the preparation to consider Map as simple type. Resolves #1676. See #1675.

Fix PersistenceConstructor of Completion to accept the property type String[]. See #1675.

See #1675.

mp911de · 2021-02-03T08:27:09Z

Review comments are addressed, commits are squashed and this one is now ready to be merged.

sothawo · 2021-02-03T19:21:33Z

I have 4 commits that the issue branch is ahead of master, the first changing the pom. I can cherry pick the 3 commits with code changes, but if I do this, how do I finish this PR?

mp911de · 2021-02-04T09:03:28Z

See https://github.com/spring-projects/spring-data-build/blob/master/CONTRIBUTING.adoc#commit-messages. Specifically, cherry-pick commits, update the commit message and close this PR/delete the branch. Does that help?

sothawo · 2021-02-04T15:38:42Z

commits cherrypicked into master

fzyzcjy · 2021-03-31T06:38:30Z

Hi, when will we have this released? Thanks!

sothawo · 2021-03-31T06:45:22Z

That's included since Feb 17th (release 4.2.0.M4). 4.2.0.RC1 will be released these days, 4.2.0.GA will robably come mid April

fzyzcjy · 2021-03-31T06:46:39Z

@sothawo Thanks!

Prepare issue branch.

d18b5af

mp911de linked an issue Feb 1, 2021 that may be closed by this pull request

Consider Document as simple type #1675

Closed

mp911de assigned sothawo Feb 1, 2021

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 1, 2021

sothawo approved these changes Feb 1, 2021

View reviewed changes

mp911de force-pushed the issue/1676 branch from 46120ae to 4c482e6 Compare February 2, 2021 09:30

sothawo approved these changes Feb 2, 2021

View reviewed changes

mp911de added 3 commits February 3, 2021 09:25

Polishing.

bb3c006

Fix PersistenceConstructor of Completion to accept the property type String[]. See #1675.

Add Map to ElasticsearchSimpleTypes.

27850e9

See #1675.

mp911de force-pushed the issue/1676 branch from 4c482e6 to 27850e9 Compare February 3, 2021 08:26

sothawo removed the status: waiting-for-triage An issue we've not yet triaged label Feb 4, 2021

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 4, 2021

sothawo closed this Feb 4, 2021

sothawo deleted the issue/1676 branch February 4, 2021 15:39

sothawo mentioned this pull request Feb 4, 2021

Consider Document as simple type #1675

Closed

mp911de mentioned this pull request Feb 16, 2021

Remove warnings when registering Spring Data Elasticsearch store converters [DATACMNS-1728] spring-projects/spring-data-commons#2150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MappingElasticsearchConverter #1677

Refactor MappingElasticsearchConverter #1677

mp911de commented Feb 1, 2021

sothawo left a comment

sothawo Feb 1, 2021

mp911de Feb 2, 2021 •

edited

Loading

sothawo Feb 1, 2021

sothawo commented Feb 2, 2021

mp911de commented Feb 2, 2021

mp911de commented Feb 2, 2021

sothawo commented Feb 2, 2021

mp911de commented Feb 2, 2021

mp911de commented Feb 2, 2021

sothawo commented Feb 2, 2021

sothawo left a comment

sothawo Feb 2, 2021

mp911de commented Feb 3, 2021

sothawo commented Feb 3, 2021

mp911de commented Feb 4, 2021

sothawo commented Feb 4, 2021

fzyzcjy commented Mar 31, 2021

sothawo commented Mar 31, 2021

fzyzcjy commented Mar 31, 2021

Refactor MappingElasticsearchConverter #1677

Refactor MappingElasticsearchConverter #1677

Conversation

mp911de commented Feb 1, 2021

sothawo left a comment

Choose a reason for hiding this comment

sothawo Feb 1, 2021

Choose a reason for hiding this comment

mp911de Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

sothawo Feb 1, 2021

Choose a reason for hiding this comment

sothawo commented Feb 2, 2021

mp911de commented Feb 2, 2021

mp911de commented Feb 2, 2021

sothawo commented Feb 2, 2021

mp911de commented Feb 2, 2021

mp911de commented Feb 2, 2021

sothawo commented Feb 2, 2021

sothawo left a comment

Choose a reason for hiding this comment

sothawo Feb 2, 2021

Choose a reason for hiding this comment

mp911de commented Feb 3, 2021

sothawo commented Feb 3, 2021

mp911de commented Feb 4, 2021

sothawo commented Feb 4, 2021

fzyzcjy commented Mar 31, 2021

sothawo commented Mar 31, 2021

fzyzcjy commented Mar 31, 2021

mp911de Feb 2, 2021 •

edited

Loading